Skip to content

[dotnet] Fix webauth credential to allow nullable rpID #15201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jan 31, 2025
Merged

[dotnet] Fix webauth credential to allow nullable rpID #15201

merged 5 commits into from
Jan 31, 2025

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 31, 2025 •
edited by qodo-merge-pro bot
Loading

User description

Motivation and Context

Fixes issue, introduced in recent versions.

https://github.com/SeleniumHQ/seleniumhq.github.io/actions/runs/13062596341/job/36448769281

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix

Description

  • Made rpId parameter nullable in Credential class.

  • Updated constructors and methods to handle nullable rpId.

  • Adjusted exception documentation to reflect nullable rpId.

Changes walkthrough 📝

Relevant files
Bug fix
Credential.cs
Allow nullable `rpId` in `Credential` class                           

dotnet/src/webdriver/VirtualAuth/Credential.cs

  • Changed rpId parameter to nullable in constructor.
  • Updated CreateNonResidentCredential and CreateResidentCredential
    methods to accept nullable rpId.
  • Adjusted exception documentation for rpId parameter.
    		• +6/-6     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 31, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Authentication Validation:
    The PR makes the relying party ID (rpId) parameter nullable without documenting the security implications. The rpId is typically used for domain validation in WebAuthn/FIDO authentication. Making it nullable could potentially allow credentials to be used across different domains if not properly validated elsewhere in the codebase.

    ⚡ Recommended focus areas for review

    Validation Check

    The PR removes rpId validation but doesn't document why rpId is now allowed to be null or what the implications are for credential security and functionality

    private Credential(byte[] id, bool isResidentCredential, string? rpId, string privateKey, byte[]? userHandle, int signCount)
{
    this.id = id ?? throw new ArgumentNullException(nameof(id));
    this.IsResidentCredential = isResidentCredential;
    this.RpId = rpId;
    this.PrivateKey = privateKey ?? throw new ArgumentNullException(nameof(privateKey));
    this.userHandle = userHandle;
    this.SignCount = signCount;
}

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 31, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add missing parameter validation

    Add validation for userHandle parameter in CreateResidentCredential method since
    it's a required parameter for resident credentials according to WebAuthn
    specification.

    dotnet/src/webdriver/VirtualAuth/Credential.cs [71-74]

     public static Credential CreateResidentCredential(byte[] id, string? rpId, string privateKey, byte[] userHandle, int signCount)
 {
+    if (userHandle == null) throw new ArgumentNullException(nameof(userHandle));
     return new Credential(id, true, rpId, privateKey, userHandle, signCount);
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical validation gap for resident credentials where userHandle is required by WebAuthn spec but currently not validated, which could lead to runtime issues. Adding this null check is essential for maintaining data integrity and spec compliance.

    		9

    RenderMichael
    RenderMichael requested changes Jan 31, 2025
    View reviewed changes
    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I could be misremembering, but I think a credential doesn't work if the rpId is null.

    I'm a little concerned about misleading users if we mark the field as nullable. In order to get rid of the null argument warning, maybe we can change FromDictionary from TryGetValue to a direct indexer.

    @nvborisenko
    Copy link
    Member Author

    nvborisenko commented Jan 31, 2025

    Found this one: https://chromedevtools.github.io/devtools-protocol/tot/WebAuthn/#type-Credential

    Seems rpId is optional, but user must set it when adding new credential. If user will get exception from remote end, it is totally fine.

    @RenderMichael
    Copy link
    Contributor

    RenderMichael commented Jan 31, 2025 •
    edited
    Loading

    What if the Create{Non}ResidentCredential methods had a non-nullable rpid parameter, but the constructor and RpId property allowed null? That way, we will warn users about setting the value, but we will not throw any exceptions ourselves.

    RenderMichael
    RenderMichael approved these changes Jan 31, 2025
    View reviewed changes
    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Per comments about rpId being optional, this change makes sense to me

    RenderMichael
    RenderMichael approved these changes Jan 31, 2025
    View reviewed changes
    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Nice! These changes offers nullability guidance to users without throwing any exceptions on the C# side

    @nvborisenko nvborisenko merged commit 74feb5d into SeleniumHQ:trunk Jan 31, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-auth-rpid branch January 31, 2025 19:04
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @nvborisenko @sandeepsuryaprasad
    [dotnet] Fix webauth credential to allow nullable rpID (SeleniumHQ#…
    5aab10c 
    …15201)
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    @nvborisenko @gryznar
    [dotnet] Fix webauth credential to allow nullable rpID (SeleniumHQ#…
    afba4a9 
    …15201)
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @RenderMichael RenderMichael RenderMichael approved these changes

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @nvborisenko @RenderMichael